feat(gateway): IPNS record response format (IPIP-351) #9399

hacdias · 2022-11-11T12:50:38Z

What

Implementation of IPIP-351: IPNS Signed Records Response Format on HTTP Gateways (ipfs/specs#351)

Checklist

Important Notes

The sharness test put round trips (#3124) was failing. I understand that it expected some output, but the test doesn't validate the output so it's not clear to me what it was. When refactoring the Routing API to the CoreAPI interface, I used the direct calls to the internal routing value store to get or put a value, instead of listening to events on the context field.

BREAKING RPC CHANGE

This changes output of selected RPC commands (https://docs.ipfs.tech/reference/kubo/rpc/):

/api/v0/dht/get and /api/v0/routing/get
/api/v0/dht/put and /api/v0/routing/put

-{
-  "Extra": "<string>",
-  "ID": "<peer-id>",
-  "Responses": [
-    {
-      "Addrs": [
-        "<multiaddr-string>"
-      ],
-      "ID": "peer-id"
-    }
-  ],
-  "Type": "<int>"
-}
+"<base64-string>"

and removes --verbose flag from all of them.

I'm not sure how critical this is. It would be great to get some feedback on why it was the way it was.

(lidel): This makes us lose the ability to see what's happening internally with the routing API for get/put with the verbose option, but since there were no tests for this we assume it is acceptable to clean this up. I suspect our DHT monitoring uses custom code anyway, and worst case, will be acceptable to refactor.

Other Notes

ipfs routing get <name> fetchs the signed IPNS key in protobuf: https://github.com/ipfs/go-ipns/blob/master/pb/ipns.proto. The output, in combination with the key, can be validated with the code below.
Routing API is not part of the CoreAPI, therefore not exposed to the gateway handlers. It is, however, part of the RPC client. Still not sure how to make it available on the gateway but shouldn't be extremely hard.
Code for validating IPNS signed records in Go and JS: https://gist.github.com/hacdias/f8935decee0c6a57513f2221c5a2fd7c

core/corehttp/gateway.go

core/corehttp/gateway_handler_ipns_record.go

core/coreapi/routing.go

hacdias · 2022-11-30T12:03:16Z

@lidel while thinking about caching of the IPNS records, I found this: ipfs/boxo#329. Now I see the following options going forward:

Keep the PR as-is, do not work on IPNS TTLs now.
Change the PR to incorporate the namesys interface update. Here I see two ways:
a. Add new function ResolveWithTTL which returns everything Resolve does + TTL. Use that for caching, and the introduced Routing API for the key.
b. Add new function ResolvePlus (with a better name) which returns everything Resolve does + TTL + IPNS Key. This way we wouldn't need to add a new Routing API. After all, when resolving the name, we have to get the key somewhere, right?

hacdias · 2022-12-01T14:15:40Z

PSA: I added a small CLI command ipfs name validate <ipns key> <record> where we can validate if record is a valid record for ipns key. It is useful for testing. Not sure if it's the best place to put it though.

core/commands/commands_test.go

core/coreapi/routing.go

lidel · 2022-12-02T13:08:08Z

incorporate the namesys interface update. [...]
Add new function ResolveWithTTL which returns everything Resolve does + TTL.

@hacdias This sounds like the best course of action. Please add it to Resolver interface.

Light clients will resolve IPNS via gateway, and that means implicit assumption the HTTP response respects the TTL set in the signed IPNS record. Reverse proxies will cache responses, so this TTL needs to be translated to HTTP Cache control headers – without this we will have flaky behavior and bad UX/DX.

If we can also fix cache-control headers for regular /ipns/ responses, that would be nice win, but I suspect gettting correct TTL for DNSLink will be more involved. To avoid time sink it is ok to descope it and focus this PR on correct TTL in application/vnd.ipfs.ipns-record responses.

ps. Unsure what you mean by "Resolve does + TTL + IPNS Key" – the key is either included in IPNS record (RSA) or inlined in the libp2p-key CID (ED25519), no need for additional lookup.

hacdias · 2022-12-02T13:16:56Z

ps. Unsure what you mean by "Resolve does + TTL + IPNS Key" – the key is either included in IPNS record (RSA) or inlined in the libp2p-key CID (ED25519), no need for additional lookup.

What I meant is that: ResolveWithTTL (or just Resolve) already has to lookup for the keys in order to resolve the address. If we first do namesys.Resolve and then Routing.Get, we are performing the lookup twice, no? My other idea (2. b.) was to have a Resolve function in namesys that also returns the key. However, that could get hairy fast as there would be too many functions doing too many different things and additional return values. Wdyt?

hacdias · 2022-12-05T09:43:00Z

Note to self: tests failing in ./t0170-legacy-dht.sh and ./t0170-routing-dht.sh for ipfs routing put. It seems ipfs dag put uses ipfs routing put.

put round trips failing. Did not understand why, will investigate later.

lidel · 2022-12-05T15:06:01Z

let's keep API changes minimal – do not change namesys APIs beyond adding ability to read TTL
if you want to return key with gateway response, you can manually add it to the record under Public Key field in protobuf – no need to change the underlying namesys API. but i dont think we need to do this – it is already present in RSA case, and is not needed for ED25519. so I'd do nothing here.
refactoring gateway code and routing commands to be DRY (use coreapi) – this code is usually old, revisited once 3-4 years, so we may need to clean things up if needed 🙈

hacdias · 2022-12-06T17:03:48Z

@lidel started working on adding the TTL to the Namesys package (ipfs/go-namesys#34). However, updating it on Kubo its being slightly harder than I expected, as we don't directly call namesys.Resolve in the HTTP handler. So I probably have to add another set of functions in Kubo in order to bubble up the TTL to the gateway. I'm not sure if doing that in this PR is a great idea as I wanted to make the scope the smallest as possible.

What we can do in this PR is:

Handle the cache of IPNS records separately because we already have the record. We can just parse it and use the TTL to manually set the headers.
Do nothing and keep it cached as the rest of the IPNS namespace.

And then solve the IPNS TTL issue in a separate PR to avoid making this PR too large and unscoped.

lidel

(1) sounds like a safer path 👍

core/commands/name/name.go

test/sharness/t0124-gateway-ipns-record.sh

core/commands/name/name.go

hacdias · 2022-12-08T11:41:40Z

Implementation ready for review with proper cache control. Note that this PR needs #9471 to be merged first.

We made changes to get/put, and there were no tests for them, so assuming they are rarely used these days. Let's mark them as Experimental to lower the expectations.

lidel

Thank you, IPIP-351 things look good 👍

I've rebased.

This PR changes the wire format of some rarely used low level commands (details below),
but since all regression tests for put and get pass I assume they are useful mostly on the CLI, and the JSON response change does not break anyone (otherwise, there would be a regression test).

I agree with your decision to switch to CoreAPI and avoid having duplicated code.
Just to be safe, I've marked them as experimental and removed unused -v parameter.

We are missing tracing span, but that can be added after we are in go-libipfs.
Since we want to unblock gateway extraction, I'm merging this as-is.

lidel · 2023-01-27T01:54:09Z

core/commands/routing.go

 		}),
 	},
-	Type: routing.QueryEvent{},
+	Type: []byte{},


💭 This changes both /api/v0/dht/get and /api/v0/routing/get (https://docs.ipfs.tech/reference/kubo/rpc/):

-{ - "Extra": "<string>", - "ID": "<peer-id>", - "Responses": [ - { - "Addrs": [ - "<multiaddr-string>" - ], - "ID": "peer-id" - } - ], - "Type": "<int>" -} +"<base64-string>"

and breaks --verbose flag in both.

I've removed flag and marked routing/get as Experimental (dht/get was already deprecated)

lidel · 2023-01-27T02:26:08Z

core/commands/routing.go

 		}),
 	},
-	Type: routing.QueryEvent{},
+	Type: []byte{},


💭 This changes both /api/v0/dht/put and /api/v0/routing/put (https://docs.ipfs.tech/reference/kubo/rpc/):

-{ - "Extra": "<string>", - "ID": "<peer-id>", - "Responses": [ - { - "Addrs": [ - "<multiaddr-string>" - ], - "ID": "peer-id" - } - ], - "Type": "<int>" -} +"<base64-string>"

and breaks --verbose flag in both.

I've removed flag and marked routing/put as Experimental (dht/put was already deprecated)

Winterhuman · 2023-01-28T20:56:59Z

@lidel Just to ask, is this feature set to arrive for Kubo v0.19 or something like v0.18.1? Not sure what the timeline for this feature is

lidel · 2023-01-28T21:54:07Z

all IPNS improvements will land in 0.19 (we want to go over proper Release Candidate first)

hacdias self-assigned this Nov 11, 2022

hacdias mentioned this pull request Nov 17, 2022

feat: add RoutingAPI to CoreAPI ipfs/interface-go-ipfs-core#91

Merged

1 task

lidel reviewed Nov 28, 2022

View reviewed changes

core/corehttp/gateway.go Outdated Show resolved Hide resolved

core/corehttp/gateway_handler_ipns_record.go Outdated Show resolved Hide resolved

core/coreapi/routing.go Outdated Show resolved Hide resolved

hacdias force-pushed the feat/gateway-ipns-records branch 2 times, most recently from df28c43 to fc8c9a2 Compare November 29, 2022 15:13

hacdias changed the title ~~feat(gateway): fetch signed IPNS records~~ feat(gateway): IPNS record response format (IPIP-351) Nov 30, 2022

hacdias force-pushed the feat/gateway-ipns-records branch from 6ab98aa to 990586f Compare November 30, 2022 12:03

hacdias mentioned this pull request Nov 30, 2022

IPIP-351: IPNS Signed Records Response Format on HTTP Gateways ipfs/specs#351

Merged

7 tasks

hacdias requested a review from lidel December 1, 2022 14:35

hacdias force-pushed the feat/gateway-ipns-records branch from ac924b8 to dde132a Compare December 2, 2022 10:20

lidel reviewed Dec 2, 2022

View reviewed changes

core/commands/commands_test.go Outdated Show resolved Hide resolved

core/coreapi/routing.go Outdated Show resolved Hide resolved

lidel requested changes Dec 6, 2022

View reviewed changes

hacdias force-pushed the feat/gateway-ipns-records branch 6 times, most recently from 1bdb9f5 to 6387e03 Compare December 8, 2022 10:49

hacdias changed the base branch from master to fix/use-ttl-on-ipns-entry December 8, 2022 10:49

hacdias requested a review from lidel December 8, 2022 10:57

hacdias force-pushed the feat/gateway-ipns-records branch from 6387e03 to 5544107 Compare December 8, 2022 11:38

hacdias force-pushed the feat/gateway-ipns-records branch from 5544107 to 6944b58 Compare December 8, 2022 11:55

hacdias force-pushed the fix/use-ttl-on-ipns-entry branch 2 times, most recently from e32eb1b to 96414b9 Compare December 13, 2022 10:31

hacdias force-pushed the feat/gateway-ipns-records branch from 915d244 to d3a9402 Compare December 13, 2022 10:54

lidel mentioned this pull request Dec 13, 2022

IPNS: support Ethereum wallet signing ipfs/specs#323

Open

hacdias force-pushed the fix/use-ttl-on-ipns-entry branch from 96414b9 to 03db314 Compare January 3, 2023 11:40

hacdias force-pushed the feat/gateway-ipns-records branch from d3a9402 to b76faa6 Compare January 3, 2023 11:41

hacdias force-pushed the fix/use-ttl-on-ipns-entry branch 2 times, most recently from 0df0d8f to 63fd9b2 Compare January 5, 2023 12:33

hacdias force-pushed the fix/use-ttl-on-ipns-entry branch from 63fd9b2 to 982ff1e Compare January 24, 2023 09:07

hacdias force-pushed the feat/gateway-ipns-records branch 3 times, most recently from 2c35c3e to ffc992d Compare January 25, 2023 09:54

hacdias force-pushed the fix/use-ttl-on-ipns-entry branch from b2ed5ae to a38adc6 Compare January 25, 2023 13:56

hacdias force-pushed the feat/gateway-ipns-records branch from ffc992d to fa6adfc Compare January 25, 2023 13:57

hacdias force-pushed the fix/use-ttl-on-ipns-entry branch 2 times, most recently from 8b48923 to 1e4c6a0 Compare January 26, 2023 16:21

hacdias force-pushed the feat/gateway-ipns-records branch from fa6adfc to aaafc4c Compare January 26, 2023 16:28

lidel mentioned this pull request Jan 27, 2023

chore: release v0.10.0 ipfs/interface-go-ipfs-core#102

Merged

Base automatically changed from fix/use-ttl-on-ipns-entry to master January 27, 2023 01:33

feat(gateway): IPNS record response format

2c319a3

lidel force-pushed the feat/gateway-ipns-records branch from aaafc4c to 2c319a3 Compare January 27, 2023 01:44

docs(rpc): routing provide, get, put

4adfd4d

We made changes to get/put, and there were no tests for them, so assuming they are rarely used these days. Let's mark them as Experimental to lower the expectations.

lidel approved these changes Jan 27, 2023

View reviewed changes

lidel merged commit a3c70a1 into master Jan 27, 2023

lidel deleted the feat/gateway-ipns-records branch January 27, 2023 03:46

bumblefudge mentioned this pull request Jan 30, 2023

IPID spec refresh wishlist did-ipid/ipid-method-spec#13

Open

lidel mentioned this pull request Jan 31, 2023

Release 0.19 #9502

Closed

BigLep mentioned this pull request Mar 9, 2023

Gateway: response type for fetching signed IPNS records ipfs/specs#320

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gateway): IPNS record response format (IPIP-351) #9399

feat(gateway): IPNS record response format (IPIP-351) #9399

hacdias commented Nov 11, 2022 •

edited by lidel

Loading

hacdias commented Nov 30, 2022

hacdias commented Dec 1, 2022

lidel commented Dec 2, 2022

hacdias commented Dec 2, 2022

hacdias commented Dec 5, 2022 •

edited

Loading

lidel commented Dec 5, 2022 •

edited

Loading

hacdias commented Dec 6, 2022 •

edited

Loading

lidel left a comment

hacdias commented Dec 8, 2022

lidel left a comment •

edited

Loading

lidel Jan 27, 2023

lidel Jan 27, 2023

Winterhuman commented Jan 28, 2023

lidel commented Jan 28, 2023 •

edited

Loading

feat(gateway): IPNS record response format (IPIP-351) #9399

feat(gateway): IPNS record response format (IPIP-351) #9399

Conversation

hacdias commented Nov 11, 2022 • edited by lidel Loading

What

Checklist

Important Notes

BREAKING RPC CHANGE

Other Notes

hacdias commented Nov 30, 2022

hacdias commented Dec 1, 2022

lidel commented Dec 2, 2022

hacdias commented Dec 2, 2022

hacdias commented Dec 5, 2022 • edited Loading

lidel commented Dec 5, 2022 • edited Loading

hacdias commented Dec 6, 2022 • edited Loading

lidel left a comment

Choose a reason for hiding this comment

hacdias commented Dec 8, 2022

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel Jan 27, 2023

Choose a reason for hiding this comment

lidel Jan 27, 2023

Choose a reason for hiding this comment

Winterhuman commented Jan 28, 2023

lidel commented Jan 28, 2023 • edited Loading

hacdias commented Nov 11, 2022 •

edited by lidel

Loading

hacdias commented Dec 5, 2022 •

edited

Loading

lidel commented Dec 5, 2022 •

edited

Loading

hacdias commented Dec 6, 2022 •

edited

Loading

lidel left a comment •

edited

Loading

lidel commented Jan 28, 2023 •

edited

Loading